-
-
Notifications
You must be signed in to change notification settings - Fork 455
Adding XAML Support and Upgrading to TurboModule for Windows #1025
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Adding XAML Support and Upgrading to TurboModule for Windows #1025
Conversation
- Implement DatePicker TurboModule with promise-based API - Implement TimePicker TurboModule with promise-based API - Add JavaScript wrapper (DateTimePickerWindows) for imperative API - Add TurboModule specs matching Android pattern - Register TurboModules via AddAttributedModules() - Update documentation with TurboModule usage examples - Export DateTimePickerWindows from main index.js This provides feature parity with Android's imperative DateTimePickerAndroid.open() API.
- Fix circular import in DateTimePickerWindows.js (was importing from itself instead of .windows file) - Change TurboModuleRegistry.getEnforcing() to get() for Windows specs to handle test environments - Update Jest snapshot to include DateTimePickerWindows export All tests now passing (22/22)
- Created TimePickerFabric.cpp/h implementing ContentIslandComponentView - Registered RNTimePickerWindows as Fabric component - Fixed DateTimePickerWindows.open() to return promise result - Updated ReactPackageProvider to register TimePicker Fabric component - Added TimePickerFabric files to vcxproj build configuration This enables XAML TimePicker control to work with React Native's new architecture (Fabric) on Windows platform.
|
@protikbiswas100 In the video attached there is no date time picker ui please check and add |
added a demo video |
- Add main.cpp with wWinMain entry point for native Win32 window creation - Remove XAML/IDL dependencies (App.xaml, MainPage.xaml, App.idl, MainPage.idl) - Update DateTimePickerDemo.vcxproj to use standard Application configuration (not UWP) - Remove AppContainerApplication and ApplicationType properties - Simplify project structure: removed UWP manifest and asset references - Update entry point from App::OnLaunched to wWinMain(HINSTANCE, HINSTANCE, PWSTR, int) - Implement WindowProc message handler for WM_DESTROY and WM_SIZE events - Initialize React Native host directly in C++ code - Use direct Windows API calls (RegisterClass, CreateWindowEx, ShowWindow) - Maintain full compatibility with DateTimePicker component and React Native Windows - Update example/index.js entry point to DateTimePickerDemo component This provides a cleaner, simpler Win32 application structure without UWP overhead, while maintaining full React Native and DateTimePicker functionality.
|
@protikbiswas100 why does the video text says "web preview" ? Can you please record with entire windows app window so that the close button is visible and update the js ui with correct text details. |
I copied and pasted the app.tsx code from some place else, I will do it afresh and reupload the video |
…ependencies - Replaced App/MainPage UWP classes with clean Win32 entry point - Changed from UWP to Win32 application type in vcxproj - Updated to use ReactNativeAppBuilder instead of XAML framework - Converted to use Composition-based CppApp props (not UWP) - Added proper Win32 headers and resource files - Removed XAML, IDL, and manifest files - Clean C++ only codebase
- Added DateTimePickerDemo.rc with version info and icon resources - Added small.ico application icon (16x16 minimal icon) - Updated resource.h with proper Win32 resource definitions - Updated vcxproj to include ResourceCompile and icon - Updated vcxproj.filters to organize resource files This completes the UWP to Win32 conversion by adding standard Win32 resource files that were removed during the migration.
|
/azp run PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs involved changes. Please handle the comments and related places and revert for a fresh code review.
protikbiswas100
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed all review comments in next commit
sundaramramaswamy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address comments.
| DatePickerComponent::DatePickerComponent() { | ||
| m_control = winrt::Microsoft::UI::Xaml::Controls::CalendarDatePicker{}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| DatePickerComponent::DatePickerComponent() { | |
| m_control = winrt::Microsoft::UI::Xaml::Controls::CalendarDatePicker{}; | |
| DatePickerComponent::DatePickerComponent() | |
| : m_control{winrt::Microsoft::UI::Xaml::Controls::CalendarDatePicker{}} { | |
| } |
Initializer lists are better to initialize members in C++. Refer Should my constructors use “initialization lists” or “assignment”?.
| DatePickerComponent::~DatePickerComponent() { | ||
| Cleanup(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class doesn't need a destructor (or the Cleanup method). Why?
unique_ptr<std::function>does nothing when it goes out of scope- Auto-revoking event tokens get called automatically when going out of scope (refer).
void f() {
{
ButtonClicked_Revoker r = button.Click(winrt::auto_revoke, [](auto const&, auto const &) { });
// yada yada
}
// `r` is out of scope. It's destructor automatically would've called `r.revoke()`.
// Revoked. Callback would no longer be called.
}This concept is recursive. When an object holding a revoker gets destroyed, its members get destroyed too. Upon destruction, auto-revoker revokes; hence the name auto-revoker.
Please drop both ~DatePickerComponent and Cleanup.
| TimePickerComponent::TimePickerComponent() { | ||
| m_control = winrt::Microsoft::UI::Xaml::Controls::TimePicker{}; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use initalizer list.
| TimePickerComponent::~TimePickerComponent() { | ||
| Cleanup(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like mentioned earlier, both Cleanup and destructor for this class are unneeded. Please drop.
|
|
||
| // Update clock format (12-hour vs 24-hour) | ||
| if (props.find("is24Hour") != props.end()) { | ||
| bool is24Hour = props["is24Hour"].AsBoolean(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| bool is24Hour = props["is24Hour"].AsBoolean(); | |
| const bool is24Hour = props["is24Hour"].AsBoolean(); |
Why? Refer What is "const correctness"?.
|
|
||
| // Update minute increment | ||
| if (props.find("minuteInterval") != props.end()) { | ||
| int32_t minuteInterval = static_cast<int32_t>(props["minuteInterval"].AsInt64()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| int32_t minuteInterval = static_cast<int32_t>(props["minuteInterval"].AsInt64()); | |
| const int32_t minuteInterval = static_cast<int32_t>(props["minuteInterval"].AsInt64()); |
| int64_t timeInMilliseconds = props["selectedTime"].AsInt64(); | ||
| auto timeInSeconds = timeInMilliseconds / 1000; | ||
| auto hours = (timeInSeconds / 3600) % 24; | ||
| auto minutes = (timeInSeconds / 60) % 60; | ||
|
|
||
| // Create TimeSpan (100-nanosecond intervals) | ||
| winrt::Windows::Foundation::TimeSpan timeSpan{ | ||
| static_cast<int64_t>((hours * 3600 + minutes * 60) * 10000000) | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| int64_t timeInMilliseconds = props["selectedTime"].AsInt64(); | |
| auto timeInSeconds = timeInMilliseconds / 1000; | |
| auto hours = (timeInSeconds / 3600) % 24; | |
| auto minutes = (timeInSeconds / 60) % 60; | |
| // Create TimeSpan (100-nanosecond intervals) | |
| winrt::Windows::Foundation::TimeSpan timeSpan{ | |
| static_cast<int64_t>((hours * 3600 + minutes * 60) * 10000000) | |
| }; | |
| const int64_t timeInMilliseconds = props["selectedTime"].AsInt64(); | |
| const auto timeInSeconds = timeInMilliseconds / 1000; | |
| const auto hours = (timeInSeconds / 3600) % 24; | |
| const auto minutes = (timeInSeconds / 60) % 60; | |
| // Create TimeSpan (100-nanosecond intervals) | |
| const winrt::Windows::Foundation::TimeSpan timeSpan{ | |
| static_cast<int64_t>((hours * 3600 + minutes * 60) * 10000000) | |
| }; |
| private: | ||
| winrt::Microsoft::UI::Xaml::XamlIsland m_xamlIsland{nullptr}; | ||
| winrt::Microsoft::UI::Xaml::Controls::TimePicker m_timePicker{nullptr}; | ||
| bool m_updating = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please refer to my comment on another file where m_updating is used.
| inline void RegisterTimePickerComponentView(winrt::Microsoft::ReactNative::IReactPackageBuilder const &packageBuilder) { | ||
| packageBuilder.as<winrt::Microsoft::ReactNative::IReactPackageBuilderFabric>().AddViewComponent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move implementation alone to TimePickerFabric.cpp. Keep the interface here.
| void TimePickerModule::Open(ReactNativeSpecs::TimePickerModuleWindowsSpec_TimePickerOpenParams &¶ms, | ||
| winrt::Microsoft::ReactNative::ReactPromise<ReactNativeSpecs::TimePickerModuleWindowsSpec_TimePickerResult> promise) noexcept { | ||
| // Clean up any existing picker | ||
| m_timePickerComponent.reset(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Combine unqiue_ptr.reset with assignment below. Refer to my other comment.
Summary
Test Plan
Created a sample DateTimePicker app to demonstrate usage, ran the app using
yarn react-native run-windows --arch x64date_time_picker_demo.mp4
What's required for testing (prerequisites)? NA
What are the steps to reproduce (after prerequisites)? NA
Compatibility
Checklist
README.mdexample/App.js)